Skip to content
This repository has been archived by the owner on Mar 12, 2020. It is now read-only.

Update palette #1215

Merged
merged 3 commits into from
Nov 21, 2018
Merged

Update palette #1215

merged 3 commits into from
Nov 21, 2018

Conversation

javamonn
Copy link
Contributor

@javamonn javamonn commented Nov 5, 2018

Updates palette to latest version.

See https://github.com/artsy/palette/pull/109/files for the associated PR on palette.

#trivial
#skip_new_tests

@DangerCI
Copy link

DangerCI commented Nov 5, 2018

Warnings
⚠️

This PR comes from a fork, and won't get JS generated for QA testing this PR inside the Emission Example app.

Generated by 🚫 dangerJS

@@ -76,14 +76,14 @@ class Artwork extends React.Component<Props, any> {
<Badges>
{is_acquireable && (
<Badge>
<Sans fontSize={8} lineHeight={8} style={{ paddingTop: 1 }} weight="medium" size="1">
<Sans fontSize="8px" lineHeight={8} style={{ paddingTop: 1 }} weight="medium" size="1">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the px hardcoding here, the values appearing in the snapshot changed dramatically from 8 to something like 72. I think this was the result of the issue described in artsy/reaction#1535.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, interesting... is it only affecting the snapshot or is the line height also affected in the UI?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is related to @mzikherman updating the badges to include an item that was outside of Palette's theme file, per @briansw's request.

Copy link
Contributor Author

@javamonn javamonn Nov 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It affects both:

Before fixing px:
screen shot 2018-11-20 at 2 18 08 pm

After:
screen shot 2018-11-20 at 2 19 00 pm

My understanding of the issue is that it only affects spots where we're passing values into fontSize and lineHeight overrides like this, though this makes me concerned about us potentially doing this in spots we don't cover with snapshot tests - I'll see if I can find any other affected spots with a search.

Edit: this appears to be the only spot we do this kind of override in Emission.

Copy link
Member

@damassi damassi Nov 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javamonn - would you mind raising this in #front-end so that others don't get tripped up accidentally?

@@ -31,7 +31,7 @@ const durationSections = (duration: Duration, labels: [string, string, string, s
]

const LabeledTimeSection: React.SFC<TimeSectionProps> = ({ time, label, timeTextProps, labelTextProps }) => (
<Flex alignItems="center" justifyConent="center">
<Flex alignItems="center" justifyContent="center">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One advantage of upgrading: this was not caught as a type error before 👏

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice one

@javamonn javamonn changed the title WIP: Try updating palette Update palette Nov 20, 2018
@@ -17,6 +17,7 @@
### Master

- Refactor auctions countdown timer, add fair countdown timer - javamonn
- Updates @artsy/palette (2.21.1) - javamonn
Copy link
Member

@damassi damassi Nov 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right on 👍 Thanks for bringing that PR over the line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

You're awesome @javamonn

@l2succes
Copy link
Contributor

merge on green

@l2succes l2succes merged commit bb48bcf into artsy:master Nov 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants